go.mod (1)

218-219: Consider documenting the temporary nature of this replace directive.

Since this replace directive is part of the race condition fix, consider moving it to the temporary replace section with a comment explaining its purpose.

 // Here are the short-lived replace from the Cosmos SDK
 // Replace here are pending PRs, or version to be tagged
-// replace (
-// 	<temporary replace>
-// )
+replace (
+    // Temporary: Using local store module for race condition fix (#22650)
+    cosmossdk.io/store => ./store
+)

CHANGELOG.md (4)

Line range hint 1-1: Add version table at the top of changelog

Consider adding a version table at the top that summarizes all v0.46.x releases with their dates and key changes for easier reference.

---

Line range hint 11-13: Fix inconsistent bullet point formatting

The bullet points use a mix of * and -. Standardize on one format throughout the document for consistency.

---

59-61: Add impact details for breaking changes

For breaking changes like the Dragonberry security fix, consider adding more details about potential impact on users and required actions.

---

Line range hint 1-1000: Fix typos and grammatical errors

Several typos and grammatical errors found throughout the document:

• "typographical" misspelled as "typograhical"
• Missing periods at end of some bullet points
• Inconsistent capitalization in section headers

store/rootmulti/store.go (1)

63-63: Naming convention for mutex variables.

Consider renaming lastCommitInfoMut to lastCommitInfoMu to align with Go conventions, where mutex variables are typically suffixed with Mu.

baseapp/baseapp.go (2)

127-127: Consider renaming stateMut to stateMutex for clarity.

According to the Uber Go Style Guide, abbreviations in variable names should be avoided to enhance readability. Renaming stateMut to stateMutex makes the purpose of the mutex clearer.

Apply this diff to improve clarity:

-	stateMut             sync.RWMutex
+	stateMutex           sync.RWMutex

---

Line range hint 583-586: Potential deadlock due to inconsistent lock ordering between app.mu and app.stateMut.

In getContextForTx, app.mu is locked before calling app.getState(mode), which in turn locks app.stateMut. If elsewhere app.stateMut is locked before attempting to acquire app.mu, this could lead to a deadlock due to inconsistent lock ordering.

Recommend reviewing the locking order to ensure that app.mu and app.stateMut are always acquired in a consistent order throughout the codebase to prevent potential deadlocks.

baseapp/abci.go (3)

609-609: Handle possible errors from CacheContext

In the assignments:

ctx, _ = app.getState(execModeFinalize).Context().CacheContext()

Ignoring the second return value without handling could overlook potential issues. Although CacheContext() may not currently return an error, future changes could introduce errors.

Consider capturing both return values explicitly for clarity:

ctx, ms := app.getState(execModeFinalize).Context().CacheContext()
// use ms if needed

Also applies to: 689-689

---

747-747: Clarify the comment for better understanding

The comment at line 747 is unclear:

// only used to handle early cancellation, for anything related to state app.getState(execModeFinalize).Context()

Consider rephrasing for clarity:

// Use ctx only for early cancellation handling. For state-related operations, use app.getState(execModeFinalize).Context()

---

1194-1194: Review the use of CacheContext

In:

ctx, _ = app.getState(execModeFinalize).Context().CacheContext()

Ensure that caching the context here is intended and that any modifications do not affect the main context unintentionally.

baseapp/abci_test.go (2)

2783-2830: Add a function comment to describe the purpose of the test

To improve code readability and maintainability, consider adding a comment before the TestABCI_Race_Commit_Query function to explain its purpose and what it is testing.

---

2796-2797: Simplify atomic counter initialization

Since the zero value of atomic.Uint64 is zero, the explicit call to counter.Store(0) is unnecessary. You can simplify the code by removing this call.

Apply the following diff:

-counter := atomic.Uint64{}
-counter.Store(0)
+var counter atomic.Uint64

Configuration used: .coderabbit.yml
Review profile: CHILL

#!/bin/bash
# Description: Verify that getState method uses proper synchronization
# Expected: Find mutex usage in getState implementation

ast-grep --pattern 'func (app *BaseApp) getState($_) $_ {
  $$$
}'

#!/bin/bash
# Description: Check for high-frequency calls to NewContextLegacy that might be affected by the added synchronization
# Expected: Identify critical paths that might need optimization

# Find direct calls to NewContextLegacy
rg "NewContextLegacy\(" -A 2

# Find potential high-frequency callers through NewContext
rg "NewContext\(" -A 2

#!/bin/bash
# Description: Check version consistency between go.mod and local store module
# Verify the store module's go.mod version matches the version specified in the main go.mod

# Get the version from main go.mod
echo "Main module store version:"
grep "cosmossdk.io/store" go.mod | head -n 1

# Get the version from local store module
echo "Local store module version:"
if [ -f "store/go.mod" ]; then
    grep "module cosmossdk.io/store" store/go.mod
fi

#!/bin/bash
# Check all local module replacements and their versions
echo "Main go.mod versions:"
grep "cosmossdk.io/" go.mod | grep -v "replace" | sort

echo -e "\nReplace directives:"
grep "replace cosmossdk.io/" go.mod | sort

echo -e "\nLocal module versions:"
for dir in api bank staking tx store; do
    if [ -f "$dir/go.mod" ]; then
        echo -e "\n$dir/go.mod:"
        grep "^module" "$dir/go.mod"
        grep "^go 1." "$dir/go.mod"
    fi
done

#!/bin/bash
# Description: Check that the project requires Go 1.19 or newer.

# Look for the 'go' directive in the go.mod file
grep "^go " go.mod

store/rootmulti/store.go (1)

514-522: Consider combining the mutex locks.

The code currently acquires and releases the mutex twice in quick succession. Consider combining these operations to reduce lock overhead:

-       rs.lastCommitInfoMut.Lock()
-       rs.lastCommitInfo = cInfo
-       rs.lastCommitInfoMut.Unlock()
-
-       rs.lastCommitInfoMut.Lock()
-       rs.lastCommitInfo.Timestamp = rs.commitHeader.Time
-       rs.lastCommitInfoMut.Unlock()
+       rs.lastCommitInfoMut.Lock()
+       rs.lastCommitInfo = cInfo
+       rs.lastCommitInfo.Timestamp = rs.commitHeader.Time
+       rs.lastCommitInfoMut.Unlock()

Configuration used: .coderabbit.yml
Review profile: CHILL

store/rootmulti/store.go (4)

63-63: Excellent use of atomic.Pointer for thread safety.

Using atomic.Pointer instead of mutex locks is a more efficient solution for this use case since the CommitInfo content itself is not modified concurrently, only the pointer is updated atomically.

This approach provides better performance than mutex locks as it:

1. Avoids lock contention
2. Allows for lock-free reads
3. Provides atomic pointer updates

---

438-443: Consider caching the atomic load result.

Multiple accesses to lastCommitInfo.Load() could be optimized by storing the result in a local variable.

-lastCommitInfo := rs.lastCommitInfo.Load()
-if lastCommitInfo == nil {
-  return GetLatestVersion(rs.db)
-}
-
-return lastCommitInfo.Version
+if info := rs.lastCommitInfo.Load(); info == nil {
+  return GetLatestVersion(rs.db)
+} else {
+  return info.Version
+}

---

510-512: Optimize multiple atomic loads.

Multiple consecutive calls to lastCommitInfo.Load() should be consolidated to a single load.

-cInfo := rs.lastCommitInfo.Load()
-cInfo.Timestamp = rs.commitHeader.Time
-
+if cInfo := rs.lastCommitInfo.Load(); cInfo != nil {
+  cInfo.Timestamp = rs.commitHeader.Time
+}

---

791-793: Consolidate atomic load operation.

The atomic load result should be cached to avoid multiple loads and null checks.

-lastCommitInfo := rs.lastCommitInfo.Load()
-if res.Height == lastCommitInfo.Version {
-  commitInfo = lastCommitInfo
+if info := rs.lastCommitInfo.Load(); info != nil && res.Height == info.Version {
+  commitInfo = info

Configuration used: .coderabbit.yml
Review profile: CHILL

CHANGELOG.md (4)

1-2: Add missing version information in title

The title should include the version number for better clarity.

- # Changelog
+ # Changelog (v0.50.x)

---

39-41: Consider adding more context to unreleased section

The unreleased section could benefit from additional context about the scope of changes and expected release timeline.

## [Unreleased]

+ This section tracks all notable changes that will be part of the next release.
+ Target release date: TBD
+
Every module contains its own CHANGELOG.md. Please refer to the module you are interested in.

---

1634-1635: Add missing security advisory details

The security advisory reference should include more details about the vulnerability and mitigation steps.

This release fixes security vulnerability identified in x/bank.
+ 
+ ## Security Advisory Details
+ - Vulnerability: [Brief description]
+ - Impact: [Description of potential impact]
+ - Mitigation: [Steps taken to fix]
+ - Affected versions: [List of versions]

---

3156-3158: Add version cutoff date for previous releases

The reference to previous releases should specify the version cutoff date for better clarity.

## Previous Releases

- [CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate).
+ [CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate, versions before v0.40.0).

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro